Skip to content

PDEP0004: implementation #49024

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 54 commits into from
Dec 13, 2022
Merged

PDEP0004: implementation #49024

merged 54 commits into from
Dec 13, 2022

Conversation

MarcoGorelli
Copy link
Member

@MarcoGorelli MarcoGorelli commented Oct 10, 2022 •
edited
Loading

Finally ready for review - I've separated the types of changes by commit to try to make the review easier. I've also split out a few changes into precursor PRs, which have now been merged.
This changes a lot of files, but I think it's hard to split it out further

PDEP for reference: https://pandas.pydata.org/pdeps/0004-consistent-to-datetime-parsing.html

@mroeschke mroeschke added the Datetime Datetime data dtype label Oct 10, 2022
@MarcoGorelli MarcoGorelli marked this pull request as ready for review October 10, 2022 18:37
@MarcoGorelli MarcoGorelli marked this pull request as draft October 13, 2022 18:22
@MarcoGorelli MarcoGorelli force-pushed the implementation-pdep-4 branch from 7c0257f to 3da20e6 Compare October 16, 2022 13:00
MarcoGorelli
MarcoGorelli commented Oct 17, 2022
View reviewed changes
@MarcoGorelli MarcoGorelli mentioned this pull request Oct 17, 2022
Merged
@MarcoGorelli MarcoGorelli force-pushed the implementation-pdep-4 branch from 69d7d72 to 23a2d36 Compare October 17, 2022 19:08
@MarcoGorelli MarcoGorelli marked this pull request as ready for review October 17, 2022 20:41
@MarcoGorelli MarcoGorelli force-pushed the implementation-pdep-4 branch from 23a2d36 to 0d2f4cb Compare October 18, 2022 08:43
@MarcoGorelli MarcoGorelli mentioned this pull request Oct 18, 2022
Closed
WillAyd
WillAyd requested changes Oct 19, 2022
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for moving this along

@MarcoGorelli MarcoGorelli force-pushed the implementation-pdep-4 branch from 0d2f4cb to 98db2b5 Compare October 19, 2022 09:25
@MarcoGorelli MarcoGorelli force-pushed the implementation-pdep-4 branch from cc307ab to ac825f5 Compare October 20, 2022 14:56
WillAyd
WillAyd reviewed Oct 28, 2022
View reviewed changes
@MarcoGorelli MarcoGorelli requested a review from Dr-Irv December 6, 2022 18:17
@MarcoGorelli MarcoGorelli added the PDEP pandas enhancement proposal label Dec 6, 2022
mroeschke
mroeschke approved these changes Dec 6, 2022
View reviewed changes
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jbrockmendel
jbrockmendel reviewed Dec 6, 2022
View reviewed changes
doc/source/user_guide/timeseries.rst

pd.to_datetime(["04-01-2012 10:00"], dayfirst=True)

pd.to_datetime(["14-01-2012", "01-14-2012"], dayfirst=True)
pd.to_datetime(["04-14-2012 10:00"], dayfirst=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this line still necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it's an example demonstrating that dayfirst isn't strict

but it's good you've highlighted this, as the blank line I'd removed was preventing it from rendering properly. now it looks fine:

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbrockmendel thanks for your review - any other objections? Sorry to tag, just hoping to move this forwards before more merge conflicts arise

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all good here. ill take another look tomorrowish if this is still up, but if it is merged before then i wont complain

WillAyd
WillAyd reviewed Dec 8, 2022
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good. One question I might just be blanking on otherwise happy to merge

WillAyd
WillAyd approved these changes Dec 8, 2022
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I say ship it

jreback
jreback reviewed Dec 9, 2022
View reviewed changes
@jreback
Copy link
Contributor

jreback commented Dec 9, 2022

just a couple of comments

MarcoGorelli
MarcoGorelli commented Dec 9, 2022
View reviewed changes
Copy link
Member Author

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jreback ! I've responded

The Py311-dev builds are failing because of #50124

@MarcoGorelli
Copy link
Member Author

Thanks all! @Dr-Irv there's still a "requested changes" from you - any further comments?

@Dr-Irv
Copy link
Contributor

Dr-Irv commented Dec 12, 2022

Thanks all! @Dr-Irv there's still a "requested changes" from you - any further comments?

I think there are 2 things that I'd like clarification on (listing them here so I don't have to find them again):

@MarcoGorelli MarcoGorelli mentioned this pull request Dec 12, 2022
Closed
Dr-Irv
Dr-Irv approved these changes Dec 12, 2022
View reviewed changes
Copy link
Contributor

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good now - all issues resolved.

@MarcoGorelli
Copy link
Member Author

I'm good now - all issues resolved.

Thanks! Really appreciate all the reviews here

That's 4 explicit approvals from core members, unless there's objections I'll merge soonish then - there's already 110+ messages here and it's getting hard to navigate

If minor objections come up I'll address them in a follow-up, and if there's any major ones we can always revert

@MarcoGorelli MarcoGorelli added this to the 2.0 milestone Dec 12, 2022
@MarcoGorelli MarcoGorelli merged commit 1d5f05c into pandas-dev:main Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noatamir noatamir noatamir left review comments

@Dr-Irv Dr-Irv Dr-Irv approved these changes

@WillAyd WillAyd WillAyd approved these changes

@jreback jreback jreback approved these changes

@mroeschke mroeschke mroeschke approved these changes

@jbrockmendel jbrockmendel Awaiting requested review from jbrockmendel

Assignees
No one assigned
Labels
Datetime Datetime data dtype PDEP pandas enhancement proposal
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.

BUG: to_datetime does not raise when errors='raise'. Inconsistent date parsing of to_datetime
7 participants
@MarcoGorelli @jreback @Dr-Irv @WillAyd @noatamir @jbrockmendel @mroeschke